OCPBUGS-15544: Add adminpolicybasedexternalroutes rights for ovnkube-node. by npinaeva · Pull Request #1867 · openshift/cluster-network-operator

npinaeva · 2023-07-05T14:27:19Z

It shouldn't change the object, therefore get, list, watch should be enough.
Initally added here #1765
Without this change non-OVN-IC deployments have errors like

current.log:2023-06-30T13:10:51.027146349Z E0630 13:10:51.027130    2903 reflector.go:148] github.com/openshift/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1/apis/informers/externalversions/factory.go:116: Failed to watch *v1.AdminPolicyBasedExternalRoute: failed to list *v1.AdminPolicyBasedExternalRoute: adminpolicybasedexternalroutes.k8s.ovn.org is forbidden: User "system:serviceaccount:openshift-ovn-kubernetes:ovn-kubernetes-node" cannot list resource "adminpolicybasedexternalroutes" in API group "k8s.ovn.org" at the cluster scope

openshift-ci · 2023-07-05T14:28:04Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

npinaeva · 2023-07-05T18:38:58Z

WIP: waiting for test results to check ovnkube-node logs, every other job without this fix has mentioned errors, e.g. https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-network-operator/1866/pull-ci-openshift-cluster-network-operator-master-e2e-gcp-ovn/1676458790074978304/artifacts/e2e-gcp-ovn/gather-extra/artifacts/pods/openshift-ovn-kubernetes_ovnkube-node-jqwlf_ovnkube-node.log

npinaeva · 2023-07-06T07:12:57Z

/retest-required

npinaeva · 2023-07-06T07:19:11Z

looks like this PR solves memory leak problem: you can pick one job from this PR e.g. https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-network-operator/1867/pull-ci-openshift-cluster-network-operator-master-e2e-azure-ovn/1676661712515764224 and same job for another pr e.g. https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-network-operator/1866/pull-ci-openshift-cluster-network-operator-master-e2e-azure-ovn/1676458790066589696
then spin up PromeCleus pod, and look at mem usage with node_namespace_pod_container:container_memory_rss{pod=~"ovnkube-.*", container="ovnkube-node"}
you will see old metrics (max 410 MB)

vs new metrics (max 60 MB)

openshift-ci-robot · 2023-07-06T07:20:48Z

@npinaeva: This pull request references Jira Issue OCPBUGS-15544, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

It shouldn't change the object, therefore get, list, watch should be enough.
Initally added here #1765
Without this change non-OVN-IC deployments have errors like

current.log:2023-06-30T13:10:51.027146349Z E0630 13:10:51.027130    2903 reflector.go:148] github.com/openshift/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1/apis/informers/externalversions/factory.go:116: Failed to watch *v1.AdminPolicyBasedExternalRoute: failed to list *v1.AdminPolicyBasedExternalRoute: adminpolicybasedexternalroutes.k8s.ovn.org is forbidden: User "system:serviceaccount:openshift-ovn-kubernetes:ovn-kubernetes-node" cannot list resource "adminpolicybasedexternalroutes" in API group "k8s.ovn.org" at the cluster scope

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jordigilh · 2023-07-06T07:21:42Z

/lgtm

npinaeva · 2023-07-06T07:22:27Z

/jira refresh

openshift-ci-robot · 2023-07-06T07:22:33Z

@npinaeva: This pull request references Jira Issue OCPBUGS-15544, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jcaamano · 2023-07-06T08:23:04Z

perhaps add it to bindata/network/ovn-kubernetes/common/007-rbac-cluster-reader.yaml as well?

npinaeva · 2023-07-06T08:27:41Z

nice catch! I didn't even know that file exists :)

jcaamano · 2023-07-06T10:04:28Z

Would this be a good time to add adminpolicybasedexternalroutes/status for the write verbs?
That's what we usually add for controllers with CRDs that update status.

jcaamano · 2023-07-06T11:23:23Z

Would this be a good time to add adminpolicybasedexternalroutes/status for the write verbs? That's what we usually add for controllers with CRDs that update status.

I guess we can do this after. PTAL @jordigilh

/lgtm
/approve

jordigilh · 2023-07-06T11:25:43Z

Would this be a good time to add adminpolicybasedexternalroutes/status for the write verbs? That's what we usually add for controllers with CRDs that update status.

I guess we can do this after. PTAL @jordigilh

/lgtm /approve

Yeah, would make sense to have only access to the status in the controller rather than the whole object. I will raise another PR. Thanks for the suggestion 😄

npinaeva · 2023-07-06T11:26:53Z

@jcaamano write verbs are only used by masters (before we default to ic) and it was added here #1765

openshift-ci-robot · 2023-07-06T11:31:04Z

/retest-required

Remaining retests: 0 against base HEAD 64ab110 and 2 for PR HEAD 0a43705 in total

jordigilh · 2023-07-06T11:33:01Z

@jcaamano write verbs are only used by masters (before we default to ic) and it was added here #1765

I guess what he means is to change this line to point to the status
https://github.com/jordigilh/cluster-network-operator/blob/cce5df4245f16688caca5b1c543376f181c0be00/bindata/network/ovn-kubernetes/common/003-rbac-controller.yaml#L90

- apiGroups: ["k8s.ovn.org"]
  resources:
  - egressfirewalls
...
...
  - "adminpolicybasedexternalroutes/status"

jcaamano · 2023-07-06T12:27:57Z

@jcaamano write verbs are only used by masters (before we default to ic) and it was added here #1765

I guess what he means is to change this line to point to the status https://github.com/jordigilh/cluster-network-operator/blob/cce5df4245f16688caca5b1c543376f181c0be00/bindata/network/ovn-kubernetes/common/003-rbac-controller.yaml#L90
- apiGroups: ["k8s.ovn.org"]
  resources:
  - egressfirewalls
...
...
  - "adminpolicybasedexternalroutes/status"

Yeah, you need to take care also in the code that you are just updating the status (edit: using the UpdateStatus method)

But it's just the same with egressips, egresssvcs, ... so we can also do this as a general effort.

edit: and we still need the read permission on the overall resource

jordigilh · 2023-07-06T14:28:46Z

@jcaamano write verbs are only used by masters (before we default to ic) and it was added here #1765

I guess what he means is to change this line to point to the status https://github.com/jordigilh/cluster-network-operator/blob/cce5df4245f16688caca5b1c543376f181c0be00/bindata/network/ovn-kubernetes/common/003-rbac-controller.yaml#L90
- apiGroups: ["k8s.ovn.org"]
  resources:
  - egressfirewalls
...
...
  - "adminpolicybasedexternalroutes/status"
Yeah, you need to take care also in the code that you are just updating the status (edit: using the UpdateStatus method)

But it's just the same with egressips, egresssvcs, ... so we can also do this as a general effort.

edit: and we still need the read permission on the overall resource

Yep, it is already using UpdateStatus().

dcbw · 2023-07-06T14:38:07Z

@npinaeva awesome that it fixes the memleak thing... can you put the details of why this happens into the commit message too though?

openshift-ci-robot · 2023-07-06T15:31:25Z

/retest-required

Remaining retests: 0 against base HEAD 329e328 and 1 for PR HEAD 0a43705 in total

It shouldn't change the object, therefore get, list, watch should be enough. The memory leak was hapenning because ovn-k doesn't fail when a controller can't watch resources, but it references other objects like pods and namespaces that have their own workqueues and can't be handled because the main watcher didn't start, thus the queues are overflowed. Add adminpolicybasedexternalroutes to cluster-reader permissions together with the other k8s.ovn.org resources Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>

npinaeva · 2023-07-06T17:06:20Z

added some memory leak details to the comment

jcaamano · 2023-07-07T07:16:42Z

/lgtm

openshift-ci · 2023-07-07T07:17:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaamano, jordigilh, npinaeva

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcaamano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-07-07T07:31:11Z

/retest-required

Remaining retests: 0 against base HEAD 660d5bc and 2 for PR HEAD 98c7509 in total

npinaeva · 2023-07-07T10:46:03Z

/retest-required

openshift-ci-robot · 2023-07-07T15:31:16Z

/retest-required

Remaining retests: 0 against base HEAD 3a5af01 and 1 for PR HEAD 98c7509 in total

dcbw · 2023-07-08T03:50:18Z

/override ci/prow/e2e-metal-ipi-ovn-ipv6
must-gather failed, not the tests

/override ci/prow/e2e-vsphere-ovn-windows

 time="2023-07-07T19:40:43Z" level=info msg="  Found ClusterServiceVersion \"openshift-windows-machine-config-operator/windows-machine-config-operator.v9.0.0\" phase: Installing"
time="2023-07-07T19:41:29Z" level=fatal msg="Failed to run packagemanifests: error waiting for CSV to install: etcdserver: request timed out\n"
Waiting for deployment "windows-machine-config-operator" rollout to finish: 0 of 1 updated replicas are available...
error: timed out waiting for the condition

/override ci/prow/e2e-gcp-ovn
Fixed by openshift/ovn-kubernetes#1747

openshift-ci · 2023-07-08T03:50:38Z

@dcbw: Overrode contexts on behalf of dcbw: ci/prow/e2e-gcp-ovn, ci/prow/e2e-metal-ipi-ovn-ipv6, ci/prow/e2e-vsphere-ovn-windows

Details

In response to this:

/override ci/prow/e2e-metal-ipi-ovn-ipv6
must-gather failed, not the tests

/override ci/prow/e2e-vsphere-ovn-windows

time="2023-07-07T19:40:43Z" level=info msg="  Found ClusterServiceVersion \"openshift-windows-machine-config-operator/windows-machine-config-operator.v9.0.0\" phase: Installing"
time="2023-07-07T19:41:29Z" level=fatal msg="Failed to run packagemanifests: error waiting for CSV to install: etcdserver: request timed out\n"
Waiting for deployment "windows-machine-config-operator" rollout to finish: 0 of 1 updated replicas are available...
error: timed out waiting for the condition

/override ci/prow/e2e-gcp-ovn
Fixed by openshift/ovn-kubernetes#1747

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-07-08T03:50:38Z

@npinaeva: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-network-mtu-migration-ovn-ipv4	`98c7509`	link	false	`/test e2e-network-mtu-migration-ovn-ipv4`
ci/prow/e2e-vsphere-ovn-dualstack	`98c7509`	link	false	`/test e2e-vsphere-ovn-dualstack`
ci/prow/e2e-ovn-hybrid-step-registry	`98c7509`	link	false	`/test e2e-ovn-hybrid-step-registry`
ci/prow/e2e-openstack-ovn	`98c7509`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-ovn-ipsec-step-registry	`98c7509`	link	false	`/test e2e-ovn-ipsec-step-registry`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-07-08T03:54:23Z

@npinaeva: Jira Issue OCPBUGS-15544: All pull requests linked via external trackers have merged:

openshift/cluster-network-operator#1867

Jira Issue OCPBUGS-15544 has been moved to the MODIFIED state.

Details

In response to this:

It shouldn't change the object, therefore get, list, watch should be enough.
Initally added here #1765
Without this change non-OVN-IC deployments have errors like

current.log:2023-06-30T13:10:51.027146349Z E0630 13:10:51.027130    2903 reflector.go:148] github.com/openshift/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1/apis/informers/externalversions/factory.go:116: Failed to watch *v1.AdminPolicyBasedExternalRoute: failed to list *v1.AdminPolicyBasedExternalRoute: adminpolicybasedexternalroutes.k8s.ovn.org is forbidden: User "system:serviceaccount:openshift-ovn-kubernetes:ovn-kubernetes-node" cannot list resource "adminpolicybasedexternalroutes" in API group "k8s.ovn.org" at the cluster scope

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2023

npinaeva marked this pull request as ready for review July 5, 2023 18:37

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2023

openshift-ci bot requested review from jcaamano and tssurya July 5, 2023 18:41

npinaeva changed the title ~~Add adminpolicybasedexternalroutes rights for ovnkube-node.~~ OCPBUGS-15544: Add adminpolicybasedexternalroutes rights for ovnkube-node. Jul 6, 2023

openshift-ci bot assigned jordigilh Jul 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 6, 2023

openshift-ci bot requested a review from anuragthehatter July 6, 2023 07:22

npinaeva force-pushed the add-apbroute-node-rbac branch from 2beadee to 0a43705 Compare July 6, 2023 08:27

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023

openshift-ci bot assigned jcaamano Jul 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2023

npinaeva force-pushed the add-apbroute-node-rbac branch from 0a43705 to 98c7509 Compare July 6, 2023 17:05

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2023

openshift-merge-robot merged commit 47c5d16 into openshift:master Jul 8, 2023

npinaeva deleted the add-apbroute-node-rbac branch July 11, 2023 08:20

openshift-ci-robot mentioned this pull request Jul 19, 2023

OCPBUGS-15544: Enable multi-external-gateway feature by default for managed and hosted clusters #1887

Merged

Conversation

npinaeva commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 5, 2023

Uh oh!

npinaeva commented Jul 5, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

openshift-ci-robot commented Jul 6, 2023

Uh oh!

jordigilh commented Jul 6, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

openshift-ci-robot commented Jul 6, 2023

Uh oh!

jcaamano commented Jul 6, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

jcaamano commented Jul 6, 2023

Uh oh!

jcaamano commented Jul 6, 2023

Uh oh!

jordigilh commented Jul 6, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

openshift-ci-robot commented Jul 6, 2023

Uh oh!

jordigilh commented Jul 6, 2023

Uh oh!

jcaamano commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordigilh commented Jul 6, 2023

Uh oh!

dcbw commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 6, 2023

Uh oh!

npinaeva commented Jul 6, 2023

Uh oh!

jcaamano commented Jul 7, 2023

Uh oh!

openshift-ci bot commented Jul 7, 2023

Uh oh!

openshift-ci-robot commented Jul 7, 2023

Uh oh!

npinaeva commented Jul 7, 2023

Uh oh!

openshift-ci-robot commented Jul 7, 2023

Uh oh!

dcbw commented Jul 8, 2023

Uh oh!

openshift-ci bot commented Jul 8, 2023

Uh oh!

openshift-ci bot commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

npinaeva commented Jul 5, 2023 •

edited

Loading

jcaamano commented Jul 6, 2023 •

edited

Loading

dcbw commented Jul 6, 2023 •

edited

Loading

openshift-ci bot commented Jul 8, 2023 •

edited

Loading